Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add back the governance module #272

Merged
merged 3 commits into from
Apr 11, 2022
Merged

Add back the governance module #272

merged 3 commits into from
Apr 11, 2022

Conversation

liamsi
Copy link
Member

@liamsi liamsi commented Apr 4, 2022 •
edited
Loading

Description

Basically copy & paste everything gov related from a fresh starport app.

related to: #168

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

liamsi
liamsi commented Apr 4, 2022
View reviewed changes
app/app.go Outdated
govRouter := govtypes.NewRouter()
govRouter.AddRoute(govtypes.RouterKey, govtypes.ProposalHandler).
AddRoute(paramproposal.RouterKey, params.NewParamChangeProposalHandler(app.ParamsKeeper)).
// TODO: do we want a community pool at all? If not, does it even make sense to include that NewCommunityPoolSpendProposalHandler?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's discuss the community pool in a separate issue/github dicsussion thread. I'll remove the todo after we have that issue/discusssion.

evan-forbes
evan-forbes approved these changes Apr 5, 2022
View reviewed changes
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay! Thanks for adding this back in!

evan-forbes
evan-forbes requested changes Apr 5, 2022
View reviewed changes
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops, sorry, reviewed my phone and got too excited. setting a block so we don't accidentally merge before we set the gov module account.

@liamsi
Copy link
Member Author

liamsi commented Apr 5, 2022

before we set the gov module account

What do you mean?

liamsi
liamsi commented Apr 5, 2022
View reviewed changes
app/app.go Show resolved Hide resolved
@liamsi liamsi mentioned this pull request Apr 6, 2022
Closed
5 tasks
@liamsi
Copy link
Member Author

liamsi commented Apr 8, 2022

I think these are all the changes necessary to add back the governance module. While modifying the code, I've realized that we could restructure files/packages slightly to make it more readable/accessible. I think we can slowly move away from the way starport generates and structures everything. But that is orthogonal to this PR.

Either way, turning this into ready for review now.

@liamsi liamsi marked this pull request as ready for review April 8, 2022 13:02
@liamsi liamsi requested a review from adlerjohn April 8, 2022 15:57
adlerjohn
adlerjohn previously approved these changes Apr 10, 2022
View reviewed changes
Copy link
Member

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@adlerjohn adlerjohn linked an issue Apr 10, 2022 that may be closed by this pull request
Decide on governance #168
Closed
5 tasks
evan-forbes
evan-forbes previously approved these changes Apr 10, 2022
View reviewed changes
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@evan-forbes
Copy link
Member

fairly certain that the linter is only failing because of what was already fixed in #273

@evan-forbes
Copy link
Member

While modifying the code, I've realized that we could restructure files/packages slightly to make it more readable/accessible.

I agree. I like what osmosis does here by separating the modules out into their own file. We could likely do this with even more components of the app.

https://github.com/osmosis-labs/osmosis/blob/main/app/modules.go

@liamsi
Copy link
Member Author

liamsi commented Apr 10, 2022

fairly certain that the linter is only failing because of what was already fixed in #273

Let me rebase to be more certain :)

I like what osmosis does here by separating the modules out into their own file. We could likely do this with even more components of the app.

Yes, exactly. I also thought Osmosis was a good example here!

@liamsi liamsi dismissed stale reviews from evan-forbes and adlerjohn via 8117d58 April 10, 2022 17:44
@liamsi liamsi force-pushed the liamsi/add-back-governance branch from 4904e71 to 8117d58 Compare April 10, 2022 17:44
@liamsi
Copy link
Member Author

liamsi commented Apr 10, 2022

fairly certain that the linter is only failing because of what was already fixed in #273

confirmed, thanks

@liamsi liamsi requested review from adlerjohn and evan-forbes April 10, 2022 17:48
liamsi
liamsi commented Apr 10, 2022
View reviewed changes
app/app.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #272 (453ba5c) into master (1850d85) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #272   +/-   ##
=======================================
  Coverage   25.74%   25.74%           
=======================================
  Files          12       12           
  Lines        1837     1837           
=======================================
  Hits          473      473           
  Misses       1326     1326           
  Partials       38       38

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67ae66f...453ba5c. Read the comment docs.

adlerjohn
adlerjohn approved these changes Apr 11, 2022
View reviewed changes
Copy link
Member

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@liamsi liamsi merged commit ba59cec into master Apr 11, 2022
@liamsi liamsi deleted the liamsi/add-back-governance branch April 11, 2022 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evan-forbes evan-forbes evan-forbes approved these changes

@adlerjohn adlerjohn adlerjohn approved these changes

Assignees

@liamsi liamsi

Labels
None yet
Projects
No open projects
Celestia Node
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Decide on governance
4 participants
@liamsi @evan-forbes @codecov-commenter @adlerjohn